Skip to content

Conversation

@henryiii
Copy link
Collaborator

@henryiii henryiii commented Apr 17, 2025

Implementation of scikit-build/dynamic-metadata#21. Mentioned in https://discuss.python.org/t/partially-dynamic-project-metadata-proposal-pre-pep/88608.

TODO: support specifying a non-dynamic field. It should not be an error if it exists already!

@henryiii henryiii force-pushed the henryiii/feat/dynamic_metadata_needs branch 3 times, most recently from 6f2b676 to 2b45a9d Compare April 17, 2025 20:13
Signed-off-by: Henry Schreiner <[email protected]>
@henryiii henryiii force-pushed the henryiii/feat/dynamic_metadata_needs branch from 2b45a9d to c2c2dd2 Compare April 17, 2025 20:28
@henryiii henryiii requested a review from Copilot April 17, 2025 20:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements a new template dynamic‐metadata plugin and adds support for dynamic_metadata_needs, addressing issue #21. Key changes include:

  • Updating tests and plugin configuration to support optional dependencies templating.
  • Introducing new functions in the template plugin and adjusting existing plugins (regex and fancy_pypi_readme) for compatibility.
  • Refactoring provider loading and dependency resolution via TopologicalSorter.

Reviewed Changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/test_dynamic_metadata.py Added tests for optional dependencies and updated expected readme output.
tests/packages/dynamic_metadata/plugin_project.toml Extended dynamic metadata configuration to include optional-dependencies with template support.
src/scikit_build_core/metadata/template.py Added template dynamic metadata processing and its corresponding dynamic_metadata_needs function.
src/scikit_build_core/metadata/regex.py Refactored to delegate formatting to _process_dynamic_metadata and reduce code duplication.
src/scikit_build_core/metadata/fancy_pypi_readme.py Updated dynamic_metadata signature and added a new function (dynamic_requires_needs).
src/scikit_build_core/metadata/init.py Extended _process_dynamic_metadata to handle additional metadata fields.
src/scikit_build_core/builder/_load_provider.py Refactored dynamic metadata loading and dependency extraction using TopologicalSorter.
src/scikit_build_core/build/metadata.py Added backward compatibility for dynamic_metadata providers and updated provider import.
pyproject.toml Added dependency for graphlib_backport and updated mypy overrides.
docs/configuration/dynamic.md Documented new templating functionality in dynamic metadata configuration.
Files not reviewed (1)
  • docs/api/scikit_build_core.metadata.rst: Language not supported
Comments suppressed due to low confidence (2)

src/scikit_build_core/build/metadata.py:53

  • [nitpick] Consider adding a comment explaining the fallback mechanism using inspect.signature for backward compatibility with providers that do not accept a metadata dict.
sig = inspect.signature(provider.dynamic_metadata)

src/scikit_build_core/builder/_load_provider.py:87

  • [nitpick] Consider adding an inline comment to clarify the purpose of converting the output of dynamic_metadata_needs into a frozenset for dependency ordering.
needs = frozenset(loaded_provider.dynamic_metadata_needs(field, config) if isinstance(loaded_provider, DynamicMetadataNeeds) else [])

@henryiii henryiii enabled auto-merge (squash) April 17, 2025 21:22
@henryiii henryiii disabled auto-merge April 18, 2025 02:53
@henryiii
Copy link
Collaborator Author

I'd like to consider an alternative, that probably can't be supported here, but could be supported by tool.dynamic-metadata: We could make these lists. So then:

[[tool.dynamic-metadata]]
field = "version"
provider = "..."

[[tool.dynamic-metadata]]
field = "readme"
provider = "..."

This would be guaranteed to always run in order top to bottom, so topological sorting is not needed. This would not have a nice path to standardization, though. A small downside is that a user then has to get the order correct, but it could be an error if they don't, and you could chain these (ideal for partially dynamic metadata).

Signed-off-by: Henry Schreiner <[email protected]>
@henryiii henryiii force-pushed the henryiii/feat/dynamic_metadata_needs branch from 2adfa37 to cbddac0 Compare April 18, 2025 20:10
@henryiii henryiii force-pushed the henryiii/feat/dynamic_metadata_needs branch 4 times, most recently from 316f1bb to f30cd58 Compare April 23, 2025 22:10
@henryiii henryiii force-pushed the henryiii/feat/dynamic_metadata_needs branch from ef33252 to 69e8424 Compare April 23, 2025 22:20
@henryiii henryiii changed the title feat: add template dynamic-metadata plugin and dynamic_metadata_needs feat: add template dynamic-metadata plugin and support requesting other fields Apr 24, 2025
@henryiii henryiii merged commit a8d15c5 into main Apr 24, 2025
64 checks passed
@henryiii henryiii deleted the henryiii/feat/dynamic_metadata_needs branch April 24, 2025 17:10
henryiii added a commit that referenced this pull request Apr 26, 2025
Also adding back a couple of test files that I wrote but missed adding
in previous PRs:

* #1051
* #1047

Signed-off-by: Henry Schreiner <[email protected]>
@bilke
Copy link
Contributor

bilke commented May 5, 2025

@henryiii Can I already use this in my project as the new functionality in dynamic-metadata is not releases yet?

@LecrisUT
Copy link
Collaborator

LecrisUT commented May 5, 2025

Technically if you require version 0.11.2 or higher, yes. The dynamic-metadata dependency is not being used, the implementation is replicated inside this project. At least it is always good to get the feedback from early adopters.

More edit: There is one point to consider is that interface of tool.scikit-build.metadata vs tool.dynamic-metadata may diverge a bit for a while when we try to allow it to be an array instead of a dict. But there should still be backwards compatibility at least.

@bilke
Copy link
Contributor

bilke commented May 5, 2025

Excerpt from my pyproject.toml:

[build-system]
requires = ["scikit-build-core", "dynamic-metadata"]
build-backend = "scikit_build_core.build"

[project]
...
dynamic = ["version", "scripts"]

[tool.scikit-build]
...
experimental = true
minimum-version = "0.11.2"
metadata.version.provider = "scikit_build_core.metadata.setuptools_scm"

But this still uses 0.1.0 of dynamic-metadata:

pip install .\[test\] -v
Using pip 24.3.1 from /Users/bilke/code/ogs/ogs/.venv/lib/python3.13/site-packages/pip (python 3.13)
Processing /Users/bilke/code/ogs/ogs
  Running command pip subprocess to install build dependencies
  Using pip 24.3.1 from /Users/bilke/code/ogs/ogs/.venv/lib/python3.13/site-packages/pip (python 3.13)
  Collecting scikit-build-core
    Obtaining dependency information for scikit-build-core from https://files.pythonhosted.org/packages/2f/ce/bed84766d74cf46620bb06162ed28157deb0f6f48440893ae3a5aa452d40/scikit_build_core-0.11.2-py3-none-any.whl.metadata
    Using cached scikit_build_core-0.11.2-py3-none-any.whl.metadata (21 kB)
  Collecting dynamic-metadata
    Obtaining dependency information for dynamic-metadata from https://files.pythonhosted.org/packages/60/53/37bb267041b21973a97bb9ea6b8e0aefbec6c7fd6db5e118c529fd27e4ba/dynamic_metadata-0.1.0-py3-none-any.whl.metadata
    Using cached dynamic_metadata-0.1.0-py3-none-any.whl.metadata (3.2 kB)
    ....

@LecrisUT
Copy link
Collaborator

LecrisUT commented May 5, 2025

dynamic-metadata is reading from tool.dynamic-metadata and scikit-build-core is reading from tool.scikit-build.metadata. I did not check what happens when both are present.

@bilke
Copy link
Contributor

bilke commented May 5, 2025

@LecrisUT Never mind, I was trying this because I got the error (without using dynamic-metadata):

scikit_build_core/builder/_load_provider.py", line 84, in _load_dynamic_metadata
      raise KeyError(msg)
  KeyError: 'scripts is not a valid field'

But it seems like the scripts field is missing: https://github.com/scikit-build/scikit-build-core/blob/main/src/scikit_build_core/metadata/__init__.py#L13-L38. I guess it should be added under _DICT_STR_FIELDS ?

@LecrisUT
Copy link
Collaborator

LecrisUT commented May 5, 2025

Hmm, indeed it seems so, but not sure if it should be scripts or entry-points for this case. The entry-points would be quite a bit more complex, and would probably mix with the support for PEP791. For now though, patching _DICT_STR_FIELDS as you said should, work, do you want to give it a try?

@henryiii
Copy link
Collaborator Author

henryiii commented May 5, 2025

This is stable/usable, it's just adding new plugins that's experimental (you'll have to set the tool.scikit-build.experimental toggle on to do that). I think our interface is fairly stable, actually, and any changes to the interface will happen inside tool.dynamic-metadata, where we don't need to worry about back-compat concerns.

henryiii added a commit that referenced this pull request May 6, 2025
Fixes issue described in
#1047 (comment).

---------

Signed-off-by: Henry Schreiner <[email protected]>
Co-authored-by: Cristian Le <[email protected]>
Co-authored-by: Henry Schreiner <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants